Skip to content

Conversation

@domhhv
Copy link
Owner

@domhhv domhhv commented Jan 27, 2026

Notes are now managed separately from occurrences, removing note embedding from occurrence objects and introducing a notesByOccurrenceId store. Occurrence-related components and services have been updated to fetch and display notes via this new mapping, improving separation of concerns and consistency. User objects now include a fetchedAt timestamp for better tracking.

Summary by CodeRabbit

  • Refactor
    • Decoupled note management from occurrence records so notes are fetched and synchronized independently.
    • Notes retrieval optimized to fetch occurrence and period notes in parallel with deduplication for more consistent note availability.
    • UI now reads notes from a centralized notes-by-occurrence map, improving note visibility across lists and forms.
    • Session/user objects include a fetchedAt timestamp to aid synchronization awareness.

✏️ Tip: You can customize this high-level summary in your review settings.

Notes are now managed separately from occurrences, removing note embedding from occurrence objects and introducing a notesByOccurrenceId store. Occurrence-related components and services have been updated to fetch and display notes via this new mapping, improving separation of concerns and consistency. User objects now include a fetchedAt timestamp for better tracking.
@domhhv domhhv self-assigned this Jan 27, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

This PR decouples notes from occurrences: occurrences no longer embed a note field. Notes are indexed by occurrence ID in a new notesByOccurrenceId map with a useNotesByOccurrenceId selector. Occurrence store shape changes to an array plus occurrencesByDate; UI and services adjusted accordingly.

Changes

Cohort / File(s) Summary
Notes store & service
src/stores/notes.store.ts, src/services/notes.service.ts
Added notesByOccurrenceId map and useNotesByOccurrenceId selector; renamed listPeriodNoteslistNotes and merged period + occurrence note fetch with dedupe and separate error handling.
Occurrence store restructuring
src/stores/occurrences.store.ts
Changed public store shape: occurrencesOccurrence[] and added occurrencesByDate map; removed setOccurrenceNote; useOccurrences now returns occurrencesByDate.
Occurrence models & services
src/models/occurrence.model.ts, src/services/occurrences.service.ts, tests/makeTestOccurrence.ts
Removed embedded note from RawOccurrence; simplified select queries to omit note relation; test fixture updated to drop note.
Occurrence UI components
src/components/occurrence/OccurrenceChip.tsx, src/components/occurrence/OccurrenceForm.tsx, src/components/occurrence/OccurrenceListItem.tsx
Replaced direct occurrence.note usage with useNotesByOccurrenceId() lookups; Form now routes add/update/delete note operations through notes store/hooks instead of setOccurrenceNote.
Note models & type guards
src/models/note.model.ts, src/utils/type-guards.ts
Exported NoteOfOccurrence type; added isNoteOfOccurrence type guard to distinguish occurrence-linked notes.
User/session shape updates
src/hooks/use-session.ts, src/services/user.service.ts, src/stores/user.store.ts
Added fetchedAt ISO timestamp to user objects returned from session/service and stored in user store (camelCased).
Tests / fixtures
tests/makeTestOccurrence.ts
Removed note property from the generated test occurrence fixture.

Sequence Diagram(s)

sequenceDiagram
  participant UI as Occurrence UI (Chip/Form/ListItem)
  participant Hook as useNotesByOccurrenceId
  participant NotesStore as notes.store
  participant NotesService as notes.service
  participant DB as Database

  UI->>Hook: request notes map for occurrenceIds
  Hook->>NotesStore: return notesByOccurrenceId[occurrenceId]
  NotesStore-->>Hook: notes map (in-memory)
  Hook-->>UI: occurrenceNote (if present)

  alt User creates/edits note via Form
    UI->>NotesStore: addNote/updateNote/deleteNote action
    NotesStore->>NotesService: call add/update/delete API
    NotesService->>DB: persist change
    DB-->>NotesService: result (note)
    NotesService-->>NotesStore: update notes & notesByOccurrenceId
    NotesStore-->>UI: updated occurrenceNote via selector
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰
I hopped through code with nimble paws,
Pulled notes out from nested jaws.
Indexed by id, neat and spry,
Occurrences breathe a lighter sigh.
Rejoice — the rabbits refactored the sky! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: notes are being managed separately from occurrences through refactored Zustand store logic.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@relativeci
Copy link

relativeci bot commented Jan 27, 2026

#173 Bundle Size — 1.82MiB (+0.05%).

bad8491(current) vs 8e3fefa main#171(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#173
     Baseline
#171
Regression  Initial JS 1.08MiB(+0.08%) 1.08MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 87.09% 100%
No change  Chunks 8 8
No change  Assets 9 9
No change  Modules 6673 6673
No change  Duplicate Modules 0 0
No change  Duplicate Code 0% 0%
No change  Packages 223 223
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#173
     Baseline
#171
Regression  JS 1.59MiB (+0.05%) 1.59MiB
No change  CSS 241.08KiB 241.08KiB

Bundle analysis reportBranch refactor/notes-by-occurrence-idProject dashboard


Generated by RelativeCIDocumentationReport issue

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/stores/notes.store.ts (1)

46-49: clearNotes should also reset notesByOccurrenceId.
Otherwise stale occurrence notes remain visible after a clear/reset.

🛠️ Proposed fix
       clearNotes: () => {
         set((state) => {
           state.notes = {};
+          state.notesByOccurrenceId = {};
         });
       },
🤖 Fix all issues with AI agents
In `@src/stores/notes.store.ts`:
- Around line 55-63: The code deletes state.notes[id] and then reads
state.notes[id], causing a runtime error; capture the note before mutation. In
the setter used by deleteNote, first assign const note = state.notes[id]; then,
if note exists and has an occurrenceId (check 'occurrenceId' in note &&
note.occurrenceId), delete state.notesByOccurrenceId[note.occurrenceId]; finally
delete state.notes[id]. Refer to state.notes, state.notesByOccurrenceId, and the
delete logic inside the set((state) => { ... }) block to locate and update the
code.

In `@src/stores/occurrences.store.ts`:
- Around line 149-155: The code updates state.occurrences correctly but still
uses the old occurredAt parameter when writing to occurrencesByDate, which
misplaces items if the API returned a new date and can throw if the date bucket
doesn't exist; update the write to use the returned updatedOccurrence.occurredAt
(via toClientOccurrence(updatedOccurrence).occurredAt or directly from
updatedOccurrence), ensure you check/create the occurrencesByDate bucket key
computed with toCalendarDate(updatedOccurredAt).toString() before assigning, and
also consider removing the entry from the old date bucket if the date changed;
update references around the map update, toClientOccurrence, occurrencesByDate
and toCalendarDate calls accordingly.
- Around line 64-74: The code in the state setter computes dateKey from
occurrence.occurredAt.split('T')[0], causing UTC-vs-local bucketing
inconsistency; change it to derive the key from the client-side object the rest
of the store uses by computing dateKey =
clientOccurrence.toCalendarDate().toString() (so addOccurrence, which updates
state.occurrences and state.occurrencesByDate using clientOccurrence and
nextOccurrence.id, will bucket identically to
fetchOccurrences/removeOccurrence/updateOccurrence).
🧹 Nitpick comments (2)
src/stores/user.store.ts (1)

75-86: Potential type inconsistency between service and store.

The updateUser service (from @services) returns camelcaseKeys(data.user, { deep: true }) without adding fetchedAt, while the store's User type expects fetchedAt: string. The code here manually adds fetchedAt to create newUser, which is the correct approach.

However, for consistency with getSession (which adds fetchedAt in the service layer), consider updating the updateUser service to also include fetchedAt in its return value. This would centralize the responsibility for adding fetchedAt in the service layer.

♻️ Optional: Add fetchedAt in service for consistency

In src/services/user.service.ts:

 export const updateUser = async (attributes: UserAttributes) => {
   const { data, error } = await supabaseClient.auth.updateUser(attributes);

   if (error) {
     throw new Error(error.message);
   }

-  return camelcaseKeys(data.user, { deep: true });
+  return camelcaseKeys(
+    { ...data.user, fetchedAt: new Date().toISOString() },
+    { deep: true }
+  );
 };

Then simplify the store:

-        const updatedSupabaseUser = await updateUser(userAttributes);
-
-        const newUser = {
-          ...updatedSupabaseUser,
-          fetchedAt: new Date().toISOString(),
-        };
+        const newUser = await updateUser(userAttributes);

         set((state) => {
           state.user = newUser;
         });

         return newUser;
src/components/occurrence/OccurrenceListItem.tsx (1)

34-36: Minor: Memoization dependency could be more precise.

The useMemo dependency array includes the entire occurrence object, but only occurrence.id is used in the computation. This may cause unnecessary recalculations if other properties of occurrence change.

♻️ Suggested improvement
   const occurrenceNote = React.useMemo(() => {
     return notes[occurrence.id];
-  }, [notes, occurrence]);
+  }, [notes, occurrence.id]);

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/stores/notes.store.ts (1)

46-49: Clear notes must also clear notesByOccurrenceId.
clearNotes leaves stale occurrence notes, so UI can still show notes after a clear.

🛠️ Proposed fix
       clearNotes: () => {
         set((state) => {
           state.notes = {};
+          state.notesByOccurrenceId = {};
         });
       },
🤖 Fix all issues with AI agents
In `@src/stores/notes.store.ts`:
- Around line 92-97: When updating state in set, you must remove any stale
mapping from notesByOccurrenceId when a note's occurrenceId changes or is
removed: read the previous note (state.notes[id]) before overwriting, if it has
an occurrenceId and it differs from updatedNote.occurrenceId delete
state.notesByOccurrenceId[oldNote.occurrenceId]; then assign
state.notes[id]=updatedNote and, if updatedNote has a truthy occurrenceId, set
state.notesByOccurrenceId[updatedNote.occurrenceId]=updatedNote; use the
existing set, notes, notesByOccurrenceId, updatedNote and id identifiers to
locate and implement this logic.
- Around line 55-61: Guard against a missing note before using the "in" operator
on noteToDelete: in the set callback where noteToDelete is assigned from
state.notes[id], first check that noteToDelete is not null/undefined, then check
for the occurrenceId property and its truthiness before deleting from
state.notesByOccurrenceId (i.e., ensure noteToDelete exists, then if
'occurrenceId' in noteToDelete and noteToDelete.occurrenceId is truthy, delete
state.notesByOccurrenceId[noteToDelete.occurrenceId]). This touches the same
block using noteToDelete and state.notesByOccurrenceId in the set(...) function
in notes.store.ts.

Comment on lines 55 to +61
set((state) => {
const noteToDelete = state.notes[id];
delete state.notes[id];

if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) {
delete state.notesByOccurrenceId[noteToDelete.occurrenceId];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard against missing note before using in.
If state.notes[id] is undefined, 'occurrenceId' in noteToDelete throws.

🛠️ Proposed fix
         set((state) => {
           const noteToDelete = state.notes[id];
           delete state.notes[id];
 
-          if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) {
+          if (noteToDelete && isNoteOfOccurrence(noteToDelete)) {
             delete state.notesByOccurrenceId[noteToDelete.occurrenceId];
           }
         });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
set((state) => {
const noteToDelete = state.notes[id];
delete state.notes[id];
if ('occurrenceId' in noteToDelete && noteToDelete?.occurrenceId) {
delete state.notesByOccurrenceId[noteToDelete.occurrenceId];
}
set((state) => {
const noteToDelete = state.notes[id];
delete state.notes[id];
if (noteToDelete?.occurrenceId) {
delete state.notesByOccurrenceId[noteToDelete.occurrenceId];
}
});
🤖 Prompt for AI Agents
In `@src/stores/notes.store.ts` around lines 55 - 61, Guard against a missing note
before using the "in" operator on noteToDelete: in the set callback where
noteToDelete is assigned from state.notes[id], first check that noteToDelete is
not null/undefined, then check for the occurrenceId property and its truthiness
before deleting from state.notesByOccurrenceId (i.e., ensure noteToDelete
exists, then if 'occurrenceId' in noteToDelete and noteToDelete.occurrenceId is
truthy, delete state.notesByOccurrenceId[noteToDelete.occurrenceId]). This
touches the same block using noteToDelete and state.notesByOccurrenceId in the
set(...) function in notes.store.ts.

Comment on lines 92 to +97
set((state) => {
state.notes[id] = updatedNote;

if ('occurrenceId' in updatedNote && updatedNote.occurrenceId) {
state.notesByOccurrenceId[updatedNote.occurrenceId] = updatedNote;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Update should remove stale occurrence mapping.
If a note changes occurrenceId (or becomes a period note), the old entry remains in notesByOccurrenceId.

🛠️ Proposed fix
         set((state) => {
-          state.notes[id] = updatedNote;
-
-          if ('occurrenceId' in updatedNote && updatedNote.occurrenceId) {
-            state.notesByOccurrenceId[updatedNote.occurrenceId] = updatedNote;
-          }
+          const previousNote = state.notes[id];
+          const nextOccurrenceId = isNoteOfOccurrence(updatedNote)
+            ? updatedNote.occurrenceId
+            : null;
+
+          state.notes[id] = updatedNote;
+
+          if (previousNote && isNoteOfOccurrence(previousNote)) {
+            if (previousNote.occurrenceId !== nextOccurrenceId) {
+              delete state.notesByOccurrenceId[previousNote.occurrenceId];
+            }
+          }
+
+          if (nextOccurrenceId) {
+            state.notesByOccurrenceId[nextOccurrenceId] = updatedNote;
+          }
         });
🤖 Prompt for AI Agents
In `@src/stores/notes.store.ts` around lines 92 - 97, When updating state in set,
you must remove any stale mapping from notesByOccurrenceId when a note's
occurrenceId changes or is removed: read the previous note (state.notes[id])
before overwriting, if it has an occurrenceId and it differs from
updatedNote.occurrenceId delete state.notesByOccurrenceId[oldNote.occurrenceId];
then assign state.notes[id]=updatedNote and, if updatedNote has a truthy
occurrenceId, set
state.notesByOccurrenceId[updatedNote.occurrenceId]=updatedNote; use the
existing set, notes, notesByOccurrenceId, updatedNote and id identifiers to
locate and implement this logic.

@domhhv domhhv merged commit ed681fa into main Jan 27, 2026
11 checks passed
@domhhv domhhv deleted the refactor/notes-by-occurrence-id branch January 27, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants